packetbeat: add support for NFS v3 and v4 protocols #1231

kofemann · 2016-03-24T17:19:43Z

This pull request is more an RFC. We will run it bit in production go get more experience. Comments are welcome. The example output:

{
  "@timestamp": "2016-03-28T06:18:18.431Z",
  "beat": {
    "hostname": "localhost",
    "name": "localhost"
  },
  "count": 1,
  "dst": "127.0.0.1",
  "dst_port": 2049,
  "nfs": {
    "minor_version": 1,
    "opcode": "GETATTR",
    "status": "NFSERR_NOENT",
    "tag": "",
    "version": 4
  },
  "rpc": {
    "auth_flavor": "unix",
    "call_size": 200,
    "cred": {
      "gid": 500,
      "gids": [
        491,
        499,
        500
      ],
      "machinename": "localhost",
      "stamp": 4597002,
      "uid": 500
    },
    "reply_size": 96,
    "status": "success",
    "time": 25631000,
    "time_str": "25.631ms",
    "xid": "2cf0c876"
  },
  "src": "127.0.0.1",
  "src_port": 975,
  "type": "nfs"
}

elasticsearch-release · 2016-03-24T17:19:44Z

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run; then say 'jenkins, test it'.

kofemann · 2016-03-24T21:42:20Z

The one of the questions I would like to know: the other beats use field names with dots, but elasticsearch 2.0 does not allow that. So is it ok in beats?

kofemann · 2016-03-28T14:28:26Z

Ok. Now it's in the shape, that others can use it as well, IOW - this is a real pull request now.

kofemann · 2016-03-28T15:21:04Z

Dashboard example

andrewkroh · 2016-03-29T00:20:21Z

packetbeat/protos/rpc/nfs.go

+	xdr   Xdr
+	vers  uint32
+	proc  uint32
+	event *common.MapStr


I would recommend not using a pointer here. Maps are reference types so they are always passed by reference.

andrewkroh · 2016-03-29T01:05:16Z

Thanks for the contribution! Very cool screen shot. A couple miscellaneous points:

Try running your package against goimports. It should change the ordering on some of the imports.
Some of the variable names are using underscores. The Go convention is mixedCase. Try running golint against your package.
I see you added some pcaps. To ensure that there are no regressions we like to have some system tests in-place that replay the pcaps and check the outputs. Could you add some test cases like these ones that use those pcaps?
And finally, to make sure that everyone understands what the fields mean it would be great if you could document their meanings. We put each field into fields.yml then we generate documentation from it by running make update.

kofemann · 2016-03-29T12:34:47Z

I have addresses all issues you pointed out.. With style it was not clear, the code used mixed style in various places, like tcptuple.Dst_ip.String(). Any way, Tests added, rebased to current master, branch updated.

andrewkroh · 2016-03-29T13:22:49Z

The one of the questions I would like to know: the other beats use field names with dots, but elasticsearch 2.0 does not allow that. So is it ok in beats?

Because field names containing dots are not allowed in ES 2.0 we do not use them in Beats. We use nested maps which gives the appearance of field names with dots because you can use dot-notation to Kibana and ES to reference the fields. For example:

event = common.MapStr{}
nfs = common.MapStr{}
event["nfs"] = nfs
nfs["version"] = 4

It appears that you figured this out. 👍 I neglected to see the question until now.

andrewkroh · 2016-03-29T14:32:42Z

packetbeat/tests/system/test_0061_nfs.py

+
+        assert o["type"] == "nfs"
+        assert o["rpc.auth_flavor"] == "unix"
+	assert "rpc.time" in o


There appears to be a few tabs here that are throwing off the indentation. Please convert them to spaces. Thanks

sorry about that. fixed.

andrewkroh · 2016-03-29T15:07:53Z

Looks good. I would like to get at least one more reviewer on this. @elastic/beats

kofemann · 2016-03-29T15:20:08Z

I will create a separate PR for dashboard later on.

andrewkroh · 2016-03-29T15:24:35Z

packetbeat/etc/fields.yml

@@ -1288,6 +1288,74 @@ trans_event:
          description: >
            The cursor identifier returned in the OP_REPLY. This must be the value that was returned from the database.

+    - name: rpc


Please run make update to re-generate the packetbeat/docs/fields.asciidoc document.

monicasarbu · 2016-03-29T15:45:41Z

@kofemann Thank you for the complete PR and the nice dashboard. I have only a small concern.

I think it would be easier for the users to understand if we rename the Packetbeat module to nfs to be the same with the name of the configuration section nfs. In this case, we can move all the RPC information from rpc under nfs.rpc.

kofemann · 2016-03-29T18:37:54Z

@monicasarbu I had the same idea during prototyping was renaming it already several times :). The reason to call it rpc is that it will be easy to add other oncrpc/sunrpc based protocols as well. Nevertheless, I am fine to rename it. @andrewkroh any strong opinion?

andrewkroh · 2016-03-29T19:38:48Z

I would rename to nfs and re-organize event structure now. If in the future new RPC based protocols are added we can refactor a bit to reuse this code.

kofemann · 2016-03-29T19:46:00Z

updated. The fields are still rpc.xxx and nfs.xxx. This makes more sense. You don't put tcp relates staff into http namspace

Signed-off-by: Tigran Mkrtchyan <[email protected]>

monicasarbu · 2016-03-30T11:08:36Z

@kofemann Ok, I agree with you that it makes more sense to have rpc and nfs inside the nfs event type.

kofemann · 2016-03-30T16:16:03Z

thanks! About dashboard: shold it go into packetbeat/etc/kibana ? Is there any conventions?

monicasarbu · 2016-03-31T08:30:17Z

@kofemann I am working now at a simple way to import and export the Kibana dashboard. For now, you can only export all the Kibana dashboards and dependencies (visualizations, searches, index-patterns) using the python script from https://github.com/elastic/beats-dashboards/tree/master/save. Please send us a PR against the beats-dashboards repo with all the exported dashboards and dependencies. I can help you to sort them out once the PR is opened. Thanks!

andrewkroh added enhancement module review Packetbeat labels Mar 29, 2016

andrewkroh reviewed Mar 29, 2016
View reviewed changes

packetbeat: add support for NFS v3 and v4 protocols

ce41d1a

Signed-off-by: Tigran Mkrtchyan <[email protected]>

monicasarbu merged commit 928d8d7 into elastic:master Mar 30, 2016

monicasarbu added a commit to monicasarbu/beats that referenced this pull request Mar 30, 2016

Add support for NFS in Packetbeat elastic#1231

1651f78

monicasarbu mentioned this pull request Mar 30, 2016

Add support for NFS in Packetbeat #1262

Merged

kofemann deleted the nfs branch March 30, 2016 16:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

packetbeat: add support for NFS v3 and v4 protocols #1231

packetbeat: add support for NFS v3 and v4 protocols #1231

kofemann commented Mar 24, 2016

elasticsearch-release commented Mar 24, 2016

kofemann commented Mar 24, 2016

kofemann commented Mar 28, 2016

kofemann commented Mar 28, 2016

andrewkroh Mar 29, 2016

kofemann Mar 29, 2016

andrewkroh commented Mar 29, 2016

kofemann commented Mar 29, 2016

andrewkroh commented Mar 29, 2016

andrewkroh Mar 29, 2016

kofemann Mar 29, 2016

andrewkroh commented Mar 29, 2016

kofemann commented Mar 29, 2016

andrewkroh Mar 29, 2016

monicasarbu commented Mar 29, 2016

kofemann commented Mar 29, 2016

andrewkroh commented Mar 29, 2016

kofemann commented Mar 29, 2016

monicasarbu commented Mar 30, 2016

kofemann commented Mar 30, 2016

monicasarbu commented Mar 31, 2016

packetbeat: add support for NFS v3 and v4 protocols #1231

packetbeat: add support for NFS v3 and v4 protocols #1231

Conversation

kofemann commented Mar 24, 2016

elasticsearch-release commented Mar 24, 2016

kofemann commented Mar 24, 2016

kofemann commented Mar 28, 2016

kofemann commented Mar 28, 2016

andrewkroh Mar 29, 2016

Choose a reason for hiding this comment

kofemann Mar 29, 2016

Choose a reason for hiding this comment

andrewkroh commented Mar 29, 2016

kofemann commented Mar 29, 2016

andrewkroh commented Mar 29, 2016

andrewkroh Mar 29, 2016

Choose a reason for hiding this comment

kofemann Mar 29, 2016

Choose a reason for hiding this comment

andrewkroh commented Mar 29, 2016

kofemann commented Mar 29, 2016

andrewkroh Mar 29, 2016

Choose a reason for hiding this comment

monicasarbu commented Mar 29, 2016

kofemann commented Mar 29, 2016

andrewkroh commented Mar 29, 2016

kofemann commented Mar 29, 2016

monicasarbu commented Mar 30, 2016

kofemann commented Mar 30, 2016

monicasarbu commented Mar 31, 2016